-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
oc: Make env use PATCH instead of PUT #5819
Conversation
I see that the response from the server after PATCH is exactly the same as the GET which means that the patch somehow isn't applied in the server. @deads2k any ideas why this happens in Origin? I tested
|
for _, info := range infos { | ||
oldData := make([][]byte, len(infos)) | ||
for i, info := range infos { | ||
old, err := json.Marshal(info.Object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this even give you versioned JSON? don't you have to use info.Codec to do this (and are you sure that will result in a patch that is valid against the client's API version?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, by using versioned objects this works like a charm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess Latest() populates VersionedObject from the API, which (maybe?) forces it to the API version the client will use? Can you test this with input from a file containing a v1beta3
object to make sure v1
is used to create the patch and submit to the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I still haven't pushed my latest changes. Latest doesn't do that ... most probably..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I will definitely test including your case
[test] |
@liggitt ptal |
UI flake re[test] |
re[test] |
failed docker-build flake rere[test] |
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7223/console
cc: @danmcp re[test] |
@@ -232,6 +234,21 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman | |||
if !one && len(resourceVersion) > 0 { | |||
return cmdutil.UsageError(cmd, "--resource-version may only be used with a single resource") | |||
} | |||
// Keep a copy of the original objects prior to updating their environment. | |||
// Used in constructing the patch(es) that will be applied in the server. | |||
oldObjects, err := resource.AsVersionedObjects(infos, outputVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outputVersion := cmdutil.OutputVersion(cmd, clientConfig.Version)
ensures we're working with the same version we're going to submit to the server, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily - clientConfig.Version becomes the default if no output
version is provided.
On Nov 19, 2015, at 11:50 AM, Jordan Liggitt notifications@github.com
wrote:
In pkg/cmd/cli/cmd/env.go
#5819 (comment):
@@ -232,6 +234,21 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
if !one && len(resourceVersion) > 0 {
return cmdutil.UsageError(cmd, "--resource-version may only be used with a single resource")
}
- // Keep a copy of the original objects prior to updating their environment.
- // Used in constructing the patch(es) that will be applied in the server.
- oldObjects, err := resource.AsVersionedObjects(infos, outputVersion)
outputVersion := cmdutil.OutputVersion(cmd, clientConfig.Version) ensures
we're working with the same version we're going to submit to the server,
right?
—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5819/files#r45364573.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should --output-version or --api-version be used to decide the version we base patches on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be using clientConfig.Version
to build oldObjects and oldData, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, forgot to switch this. Updated.
[test] |
@liggitt tests added, ptal |
@@ -16,8 +16,14 @@ oc create -f test/integration/fixtures/test-deployment-config.json | |||
oc describe deploymentConfigs test-deployment-config | |||
[ "$(oc get dc -o name | grep 'deploymentconfig/test-deployment-config')" ] | |||
[ "$(oc describe dc test-deployment-config | grep 'deploymentconfig=test-deployment-config')" ] | |||
# Patch a nil list | |||
oc env dc/test-deployment-config TEST=value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add checks to make sure the env command 1) actually took effect and 2) didn't change anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did 1, not sure how to check for 2
@@ -31,6 +31,19 @@ function os::cmd::expect_success_and_text() { | |||
os::cmd::internal::expect_exit_code_run_grep "${cmd}" "os::cmd::internal::success_func" "${expected_text}" | |||
} | |||
|
|||
# expect_success_and_multiple_text runs the cmd and expects an exit code of 0 | |||
# as well as running a grep test to find the given strings in the output | |||
# TODO: Run the command once and not for every string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to run once, grep multiple before this can merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the todo message
or did you mean to actually implement it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually implement it... we can't assume the command is side-effect free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you don't want to implement it now, that's fine, just run multiple expect_success_and_text calls in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, updated to run multiple calls
@liggitt anything else tbd here? |
if len(outputFormat) != 0 { | ||
objects, err := resource.AsVersionedObjects(infos, outputVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only place outputVersion should be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
Never seen this before (#6137)
|
re[test] |
Yuck, time zone bug. Tests will fail. Spawn an issue. On Dec 1, 2015, at 5:42 AM, Michail Kargakis notifications@github.com Never seen this before --- FAIL: TestTranslateTimestamp (0.00s) — |
Evaluated for origin test up to 0c57b08 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7569/) |
@liggitt can we merge this now? |
yup, LGTM |
[merge] |
Mentioned API version negotiation issue in #5216 (comment) |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4207/) (Image: devenv-rhel7_2851) |
Evaluated for origin merge up to 0c57b08 |
Fixes #5287
Fixes #5816